Use internal Url struct in favor of url::Url to minimize the url dep in payjoin by benalleng · Pull Request #1377 · payjoin/rust-payjoin

benalleng · 2026-03-02T20:18:33Z

This adds the internal Url Struct to no longer depend on the url crate directly for it.

However we still have reqwest inside payjoin url from payjoin-directory and test-utils which pull in the url dep, but that is only available on the io feature.

I have also added a fuzz target, which already helped me better shape the parse function better.

I have only ran the fuzzer for a limited amount of time however and some more cpu hours might be helpful

Claude really helped me get through the brunt of this.

You can test the lack of an accessible url dep not including the io feature with this command.

cargo tree -p payjoin --no-default-features --features v1,v2,directory,_manual-tls,_test-utils -e no-dev,no-build -i url

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2026-03-02T20:22:44Z

Coverage Report for CI Build 24522598976

Coverage increased (+0.4%) to 84.741%

Details

Coverage increased (+0.4%) from the base build.
Patch coverage: 36 uncovered changes across 8 files (513 of 549 lines covered, 93.44%).
3 coverage regressions across 2 files.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v1.rs	19	1	5.26%
payjoin/src/core/url.rs	463	453	97.84%
payjoin-cli/src/app/config.rs	6	4	66.67%
payjoin/src/core/io.rs	3	1	33.33%
payjoin-cli/src/app/v2/ohttp.rs	6	5	83.33%
payjoin/src/core/ohttp.rs	3	2	66.67%
payjoin/src/core/receive/error.rs	1	0	0.0%
payjoin/src/core/receive/optional_parameters.rs	25	24	96.0%

Coverage Regressions

3 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
payjoin/src/core/into_url.rs	2	94.74%
payjoin-cli/src/app/v1.rs	1	64.71%

Coverage Stats


Relevant Lines:	13271
Covered Lines:	11246
Line Coverage:	84.74%
Coverage Strength:	403.4 hits per line

💛 - Coveralls

DanGould

We can edit the spec to say "the host component of any URL in a BIP77 message MUST be an ASCII LDH (Letters, Digits, Hyphens) plus dots: [a-zA-Z0-9.-] hostname or an IPv4 address literal" to kill IDNA once and for all. For the full URL, we restrict the character set to RFC 3986 unreserved plus the structural delimiters we actually need (:/, ?, #, @). Reject any byte outside printable ASCII (0x21–0x7E) that isn't percent-encoded.

There's a specific URL fuzzer we might consider here: https://github.com/orangetw/Tiny-URL-Fuzzer https://github.com/orangetw/Tiny-URL-Fuzzer/blob/master/samples.txt could also test against

https://payjo.in\@evil.com/path
https://payjo.in%40evil.com/path
https://payjo.in#\@evil.com
https://payjo.in/../evil.com
https://payjo.in/%2e%2e/evil.com
https://evil.com:443@payjo.in/
https://payjo.in%00.evil.com/path
https://payjo.in:@evil.com/

not that these are reall attacks but just so we knwo we parse the same as url for stuff we could actually encounter. Low prio but I wanted to document.

DanGould · 2026-04-14T07:12:05Z

+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[allow(dead_code)]


note to self that this is removed by the time the PR's last commit comes around.

Do we want support for ipv4 or ipv6 hosts in payjoin::Url if not we can just keep Host::Domain.

I think for testing especially this is still useful. What do you think?

DanGould · 2026-04-14T08:14:30Z

Tnull says "Def. no huge blocker if there is a path to dropping it!" but this seems pretty darn close so I'd like to close it out to keep the prs flowing

This commit migrates the monorepo away from the external Url dep to use the new internal Url. Additionally due to the transition we need to add a dep for url encoding with `percent-encoding-rfc3986` which coincidentally get us inline with the bitcoin_uri crate.

xstoicunicornx

Have a bit of feedback.

One other thing, seems like we are replicating the url::form_urlencoded::parse method in two different places so maybe its worth creating a native implementation of this in url.rs?

xstoicunicornx · 2026-04-17T07:23:47Z

+
+    pub fn query(&self) -> Option<&str> { self.query.as_deref() }
+
+    pub fn set_query(&mut self, query: Option<&str>) {


Since this is a public fn it feels a little weird to have no validation done. Maybe just have a clear_query to easily set query to None and otherwise force usage of query_pairs_mut? Or reduce visibility to pub(crate)?

xstoicunicornx · 2026-04-17T07:28:00Z

+
+    pub fn join(&self, segment: &str) -> Result<Url, ParseError> {
+        // If the segment is a full URL (scheme://...), parse it independently.
+        // Only treat it as a full URL if :// appears before any / (i.e. in scheme position).


Nit: this comment was confusing to me when I originally read it

Suggested change

// Only treat it as a full URL if :// appears before any / (i.e. in scheme position).

// Only treat it as a full URL if no / appears before :// (i.e. in scheme position).

xstoicunicornx · 2026-04-17T07:31:22Z

+                scheme.push(c);
+            }
+            ':' => break,
+            _ => return Err(ParseError::InvalidCharacter),


Maybe just use ParseError::InvalidScheme instead, as this is more consistent with how parse_port errors are handled

xstoicunicornx · 2026-04-17T08:33:57Z

+    let mut path = String::new();
+    let mut query: Option<String> = None;
+    let mut fragment: Option<String> = None;
+
+    if let Some(frag_pos) = input.find('#') {
+        let before_fragment = &input[..frag_pos];
+        fragment = Some(input[frag_pos + 1..].to_string());
+
+        if let Some(q_pos) = before_fragment.find('?') {
+            path.push_str(&before_fragment[..q_pos]);
+            query = Some(before_fragment[q_pos + 1..].to_string());
+        } else {
+            path.push_str(before_fragment);
+        }
+    } else if let Some(q_pos) = input.find('?') {
+        path.push_str(&input[..q_pos]);
+        query = Some(input[q_pos + 1..].to_string());
+    } else {
+        path.push_str(input);
+    }


This is a bit confusing to follow why not just:

Suggested change

let mut path = String::new();

let mut query: Option<String> = None;

let mut fragment: Option<String> = None;

if let Some(frag_pos) = input.find('#') {

let before_fragment = &input[..frag_pos];

fragment = Some(input[frag_pos + 1..].to_string());

if let Some(q_pos) = before_fragment.find('?') {

path.push_str(&before_fragment[..q_pos]);

query = Some(before_fragment[q_pos + 1..].to_string());

} else {

path.push_str(before_fragment);

}

} else if let Some(q_pos) = input.find('?') {

path.push_str(&input[..q_pos]);

query = Some(input[q_pos + 1..].to_string());

} else {

path.push_str(input);

}

let (before_fragment, fragment) = match input.find('#') {

Some(pos) => (&input[..pos], Some(input[pos + 1..].to_string())),

None => (&input[..], None),

};

let (path, query) = match before_fragment.find('?') {

Some(pos) =>

(before_fragment[..pos].to_string(), Some(before_fragment[pos + 1..].to_string())),

None => (before_fragment.to_string(), None),

};

xstoicunicornx · 2026-04-17T08:42:34Z

+            Some(ref h) if h.is_empty() => return Err(ParseError::EmptyHost),
+            Some(Host::Domain(ref d))
+                if !d.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '.') =>
+                return Err(ParseError::InvalidHost),


Wouldn't it make more sense for these parsing errors to be caught in parse_host (would also be more consistent with the other parsing functions)?

Also, if we are erroring for ParseError::EmptyHost why does the host need to be an Option<Host> rather than just Host? This validation seems to ensure that the host will always exist. Not using an Option<Host> would also eliminate the need for has_host.

benalleng changed the title ~~Url rewrite~~ Remove url dep from payjoin crate* Mar 2, 2026

benalleng changed the title Remove url dep from payjoin crate* Remove url dep from payjoin crate Mar 2, 2026

benalleng changed the title ~~Remove url dep from payjoin crate~~ Use internal Url struct in favor of url::Url to minimize the url dep in payjoin Mar 2, 2026

benalleng force-pushed the url-rewrite branch from 9f303e1 to a8625de Compare March 2, 2026 21:21

benalleng mentioned this pull request Mar 2, 2026

Standup Input: Week of 2026-03-02 #1372

Closed

benalleng marked this pull request as draft March 2, 2026 23:31

This comment was marked as resolved.

Sign in to view

benalleng mentioned this pull request Mar 27, 2026

Add Payjoin Receiver Support (BIP 77) lightningdevkit/ldk-node#746

Open

benalleng force-pushed the url-rewrite branch 2 times, most recently from 7c97b6c to 8e51345 Compare March 27, 2026 15:10

benalleng marked this pull request as ready for review March 27, 2026 15:42

DanGould reviewed Apr 14, 2026

View reviewed changes

benalleng force-pushed the url-rewrite branch 12 times, most recently from 652f4a7 to 4fca182 Compare April 14, 2026 20:15

benalleng mentioned this pull request Apr 14, 2026

Remove unneeded http feature via the bhttp dep in payjoin-mailroom #1481

Merged

2 tasks

benalleng force-pushed the url-rewrite branch 3 times, most recently from 96f3353 to b3bcf75 Compare April 15, 2026 13:42

benalleng mentioned this pull request Apr 15, 2026

Should the _manual-tls feature include enabling reqwest? #1483

Closed

benalleng force-pushed the url-rewrite branch 7 times, most recently from 6efecbe to 04d22c6 Compare April 16, 2026 15:01

benalleng added 4 commits April 16, 2026 12:46

Add internal Url type to replace url crate

3235beb

Add url fuzz target

536d18b

Fixup add Url docs

29b6648

benalleng force-pushed the url-rewrite branch from 2441088 to 29b6648 Compare April 16, 2026 16:46

benalleng requested a review from DanGould April 16, 2026 16:46

xstoicunicornx reviewed Apr 17, 2026

View reviewed changes


		pub fn query(&self) -> Option<&str> { self.query.as_deref() }

		pub fn set_query(&mut self, query: Option<&str>) {

	// Only treat it as a full URL if :// appears before any / (i.e. in scheme position).
	// Only treat it as a full URL if no / appears before :// (i.e. in scheme position).

Conversation

benalleng commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 24522598976

Coverage increased (+0.4%) to 84.741%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

This comment was marked as resolved.

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanGould Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

benalleng Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DanGould commented Apr 14, 2026

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benalleng commented Mar 2, 2026 •

edited

Loading

coveralls commented Mar 2, 2026 •

edited

Loading

benalleng Apr 14, 2026 •

edited

Loading